Add common ring configuration #4617

trevorwhitney · 2021-11-02T22:52:38Z

What this PR does / why we need it:

This work finished what @DylanGuedes started with the common ring config and replaces #4610, which has a much better description.

Checklist

Documentation added
Tests updated
Add an entry in the CHANGELOG.md about the changes.

Co-Authored-By: Dylan Guedes <djmgguedes@gmail.com>

… present

trevorwhitney · 2021-11-02T23:00:30Z

pkg/loki/config_wrapper.go

+	// append the loopback after checking if the default is enabled, as it mutates the current
+	appendLoopbackInterface(r, defaults)
+
+	numTokens := 128 //this is the default used by the ingester and ruler


@owen-d per your comment on @DylanGuedes's original PR, do you think we should use 512 here instead?

I don't think we should be overwriting these at all. Extra fields (i.e. num_tokens) can still be reconfigured by setting the ingester ring tokens manually, but I don't think we should be overriding them here.

ok, so just leave that one blank for now, I'm in agreement with that.

trevorwhitney · 2021-11-02T23:02:26Z

pkg/loki/config_wrapper.go

-	r.Ingester.LifecyclerConfig.TokensFilePath = f
+func useCommonRingConfig(r, defaults *ConfigWrapper) {
+	// if the default ingester config is detected, then it's safe to override with the common config
+	shouldOverrideIngester := reflect.DeepEqual(r.Ingester.LifecyclerConfig, defaults.Ingester.LifecyclerConfig)


@owen-d per your comment on @DylanGuedes's original PR, would you like to see this DeepEqual check applied to all ring overrides? The reason why we may not need to do that is because after this ApplyDynamicConfig function runs, we the parse the config file and command line args again. So, if someone is not using the default, they must have provided configs in one of those 2 places, and thus they will get applied and overrides any dynamic configs we applied.

That's a good point. I don't think it's necessary 👍

Hey, since in such a scenario the configs would be applied again, maybe this whole shouldOverrideIngester isn't necessary?

hmm, and just always override the ingester. that's a good point. I believe we have test coverage for that case so I'll try it out

slim-bean · 2021-11-02T23:25:08Z

pkg/util/ring_config.go

@@ -82,6 +82,27 @@ func (cfg *RingConfig) ToLifecyclerConfig(numTokens int) (ring.BasicLifecyclerCo
 	}, nil
 }

+func CortexLifecyclerConfigToRingConfig(cfg ring.LifecyclerConfig) RingConfig {


I think this should be dskit instead of cortex?

I think we will do that eventually, but we've not yet migrated to the dskit ring.

owen-d

Left some initial feedback, thanks for picking this up!

owen-d · 2021-11-03T01:32:43Z

pkg/loki/config_wrapper.go

+	// append the loopback after checking if the default is enabled, as it mutates the current
+	appendLoopbackInterface(r, defaults)
+
+	numTokens := 128 //this is the default used by the ingester and ruler


I don't think we should be overwriting these at all. Extra fields (i.e. num_tokens) can still be reconfigured by setting the ingester ring tokens manually, but I don't think we should be overriding them here.

owen-d · 2021-11-03T01:34:56Z

pkg/loki/config_wrapper.go

-	r.Ingester.LifecyclerConfig.TokensFilePath = f
+func useCommonRingConfig(r, defaults *ConfigWrapper) {
+	// if the default ingester config is detected, then it's safe to override with the common config
+	shouldOverrideIngester := reflect.DeepEqual(r.Ingester.LifecyclerConfig, defaults.Ingester.LifecyclerConfig)


That's a good point. I don't think it's necessary 👍

owen-d · 2021-11-03T01:50:29Z

pkg/loki/config_wrapper.go

-func applyIngesterRingConfig(r *ConfigWrapper, defaults *ConfigWrapper) error {
-	if reflect.DeepEqual(r.Ingester.LifecyclerConfig.InfNames, defaults.Ingester.LifecyclerConfig.InfNames) {
-		appendLoopbackInterface(r)
+// 1. Gives preference to any explicit ring config set. For instance, if the user explicitly configures Distributor's ring,


I think it should be explicit > common > memberlist. Using the ingester's ring was a stopgap put in place recently and meant to accomplish what the new common.ring will, so it's not something we'll need to retain.

Agreed. I can imagine users being confused due to their different rings not using default values listed in the documentation (instead, such rings would be using the ingester config, which they might not even realize).

pkg/loki/config_wrapper.go

pkg/loki/config_wrapper_test.go

pkg/loki/config_wrapper.go

DylanGuedes · 2021-11-03T11:44:09Z

pkg/loki/config_wrapper.go

+	ingesterRingCfg := util.CortexLifecyclerConfigToRingConfig(r.Ingester.LifecyclerConfig)
+	numTokens := r.Ingester.LifecyclerConfig.NumTokens
+
+	// we're using the ingester config here, so never need to override it


Hey, I was thinking since you are passing the ingester, wouldn't it be fine to allow overriding the ingester? In such a scenario you'd be overriding the ingester with the ingester itself, so looks fine.

edit: In conclusion, maybe we can remove the whole third param

yeah, I like that idea, I'm going to look into removing the whole third param

DylanGuedes · 2021-11-03T11:50:06Z

Thank you for picking this one Trevor, and great work with the memberlist integration. 😄

I left a few nitpicking comments but LGTM.

owen-d · 2021-11-03T15:25:07Z

pkg/loki/config_wrapper.go

+	} else {
+		// neither common ring nor memberlist set, use ingester ring configuration for all rings
+		useIngesterRingConfig(r, defaults)
 	}


I don't think we should apply the ingester configs to any other ring.

If we make the decision, which I agree is more straight forward, we run the risk of 2.4 being a breaking release to people with existing configs, as it introduces a few new rings that are required

Co-authored-by: Dylan Guedes <djmgguedes@gmail.com>

DylanGuedes · 2021-11-03T19:55:19Z

pkg/loki/config_wrapper.go

@@ -76,11 +77,15 @@ func (c *ConfigWrapper) ApplyDynamicConfig() cfg.Source {
 			r.QueryScheduler.UseSchedulerRing = true
 		}

-		applyPathPrefixDefaults(r, defaults)
-		if err := applyIngesterRingConfig(r, &defaults); err != nil {
+		applyPathPrefixDefaults(r, &defaults)


Btw, since applyPathPrefixDefaults is being invoked before applyDynamicRingConfigs, the check reflect.DeepEqual(r.CompactorConfig.CompactorRing, defaults.CompactorConfig.CompactorRing) will always be false I think

edit: my bad, I meant applyTokensFilePath instead of applyPathPrefixDefaults

does applyPathPrefixDefaults modify the compactor's ring? I see it modifying the working directory, but I'm missing the impact on the ring?

Sorry, I completely confused the function names 😂 I was referring to applyTokensFilePath

owen-d

Nice work navigating that complexity, LGTM

trevorwhitney and others added 3 commits November 2, 2021 16:43

add ring section to common config

60a2c4e

Co-Authored-By: Dylan Guedes <djmgguedes@gmail.com>

fix compactor test to append loopback interface it ifnames in test if…

afd361a

… present

add loopback interface to compactor ring iface names

b59bb0d

trevorwhitney requested a review from a team as a code owner November 2, 2021 22:52

pull-request-size bot added the size/L label Nov 2, 2021

trevorwhitney changed the title ~~Common ring config~~ Add common ring configuration Nov 2, 2021

trevorwhitney mentioned this pull request Nov 2, 2021

WIP: Implement a common ring config #4610

Closed

3 tasks

trevorwhitney commented Nov 2, 2021

View reviewed changes

slim-bean reviewed Nov 2, 2021

View reviewed changes

owen-d reviewed Nov 3, 2021

View reviewed changes